Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(react-utilities): make dictionaries strict and get rid of unused generics to improve DX and type safety #23019

Draft
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

Hotell
Copy link
Contributor

@Hotell Hotell commented May 16, 2022

Current Behavior

react-utilities exposes helper functions used almost in every v9 component that:

  • implements strict runtime dictionaries which types are widened to non strict type alternative
  • are not typed properly, thus allowing to introduce any kind of issues to consumers
  • violate rule of misused generics

New Behavior

react-utilities exposes helper functions used almost in every v9 component that:

  • implements strict runtime dictionaries which produce strict types
  • are strictly typed
  • don't violate rule of misused generics

To prevent these issues , react-utilities adds lint rule that will catch those 🔥

Demo

Strict property maps via toObjectMap

// BEFORE

// $ExpectType number | ✅
const test = baseElementEvents.onChange
// $ExpectType number | 🚨 - this is wrong as "helloDarkness" doesn't exist
const test = baseElementEvents.helloDarkness

// AFTER

// $ExpectType 1  ✅
const test = baseElementEvents.onChange
// $ExpectError - ✅ Property 'helloDarkness' does not exist on type
const test = baseElementEvents.helloDarkness

getNativeProps

  • exclude props
// BEFORE

// $ExpectType {a: number, b: number} 🚨 - invalid return type
const test = getNativeProps({ a: 1, b: 2 }, ['a', 'b'], ['b']);
// $ExpectType {a: number, b: number} 🚨 - invalid exclude, invalid return type
const test = getNativeProps({ a: 1, b: 2 }, ['a', 'b'], ['hello']);

// AFTER

// $ExpectType {a: number} ✅
const test = getNativeProps({ a: 1, b: 2 }, ['a', 'b'], ['b']);
// $ExpectError ✅ - "Type '"hello"' is not assignable to type '"a" | "b" | "c"'"
const test = getNativeProps({ a: 1, b: 2 }, ['a', 'b'], ['hello']);
  • exclude props DX:

image

getNativeElementProps

// BEFORE

// $ExpectType {} - 🚨 bottom type - no type safety
const test = getNativeElementProps('div', { id: '123', checked: true }))
// $ExpectType {} - 🚨 bottom type - no type safety
const test = getNativeElementProps('div', { id: '123', checked: true }, ['checked']))
// $ExpectType {} - 🚨 No TS error although "IhaveNoIdeaWhyIamAllowedHere" is invalid prop to exclude
const test = getNativeElementProps('div', { id: '123', checked: true }, ['IhaveNoIdeaWhyIamAllowedHere']))


// AFTER

// $ExpectType JSX.IntrinsicElement['div'] - ✅ no `checked` present
const test = getNativeElementProps('div', { id: '123', checked: true, className: 'hello' }))
// $ExpectType Omit<JSX.IntrinsicElement['div'],'className'> - ✅ no `checked` + 'className' present
const test = getNativeElementProps('div', { id: '123', checked: true, className: 'hello' }, ['className']))
// $ExpectError - ✅  Type '"IhaveNoIdeaWhyIamAllowedHere"' is not assignable to type '"ref" | "as" | "key" | keyof HTMLAttributes<HTMLDivElement> 
const test = getNativeElementProps('div', { id: '123', checked: true }, ['IhaveNoIdeaWhyIamAllowedHere']))
  • exclude props + DX

image

getPartitionedNativeProps

// BEFORE

/** 
$ExpectType { 
    root: {style?: CSSProperties; className?: string ;},
    primary: 🚨 wrong - {style?: CSSProperties; className?: string; id: string, dir: string, defaultChecked: boolean } 
} 🚨
*/
const test = getPartitionedNativeProps({
      primarySlotTagName: 'div',
      props: { className: 'hello', style: { width: '100px' }, id: '123', dir: 'ltr', defaultChecked: false },
    });


/** 
$ExpectType { 
    root: {style?: CSSProperties; className?: string ;},
    primary: 🚨 wrong - {style?: CSSProperties; className?: string;, dir: string} 
} 🚨
*/
const test = getPartitionedNativeProps({
      primarySlotTagName: 'div',
      props: { className: 'hello', style: { width: '100px' }, id: '123', dir: 'ltr', defaultChecked: false },
      excludedPropNames: ['id', 'defaultChecked'],
    });

/** 
🚨 - should not error as `lang` is a prop on native `div` - Type '"lang"' is not assignable to type '"id" | "style" | "className" | "defaultChecked" | "dir"'
*/
const test = getPartitionedNativeProps({
      primarySlotTagName: 'div',
      props: { className: 'hello', style: { width: '100px' }, id: '123', dir: 'ltr', defaultChecked: false },
      excludedPropNames: ['id', 'defaultChecked', 'lang'],
    });



// AFTER

/** 
$ExpectType { 
    root: {style?: CSSProperties; className?: string ;},
    primary: Omit<JSX.IntrinsicElements['div'],'style' | 'className'>
} ✅
*/
const test = getPartitionedNativeProps({
      primarySlotTagName: 'div',
      props: { className: 'hello', style: { width: '100px' }, id: '123', dir: 'ltr', defaultChecked: false },
    });


/** 
$ExpectType { 
    root: {style?: CSSProperties; className?: string ;},
    primary: Omit<JSX.IntrinsicElements['div'], 'style' | 'className' | 'id' | 'defaultChecked' >
} ✅
*/
const test = getPartitionedNativeProps({
      primarySlotTagName: 'div',
      props: { className: 'hello', style: { width: '100px' }, id: '123', dir: 'ltr', defaultChecked: false },
      excludedPropNames: ['id', 'defaultChecked'],
    });

/** 
$ExpectType { 
    root: {style?: CSSProperties; className?: string ;},
    primary: Omit<JSX.IntrinsicElements['div'], 'style' | 'className' | 'id' | 'defaultChecked' | 'lang >
} ✅ - no error
*/
const test = getPartitionedNativeProps({
      primarySlotTagName: 'div',
      props: { className: 'hello', style: { width: '100px' }, id: '123', dir: 'ltr', defaultChecked: false },
      excludedPropNames: ['id', 'defaultChecked', 'lang'],
    });

…unused generics to improve DX and type safety
@fabricteam
Copy link
Collaborator

fabricteam commented May 16, 2022

📊 Bundle size report

Package & Exports Baseline (minified/GZIP) PR Change
react-accordion
Accordion (including children components)
73.734 kB
22.498 kB
73.769 kB
22.5 kB
35 B
2 B
react-badge
Badge
21.002 kB
6.628 kB
21.035 kB
6.644 kB
33 B
16 B
react-badge
CounterBadge
21.915 kB
6.933 kB
21.95 kB
6.948 kB
35 B
15 B
react-badge
PresenceBadge
22.246 kB
6.672 kB
22.281 kB
6.679 kB
35 B
7 B
react-divider
Divider
15.55 kB
5.595 kB
15.579 kB
5.599 kB
29 B
4 B
react-image
Image
10.235 kB
4.036 kB
10.268 kB
4.044 kB
33 B
8 B
react-label
Label
8.546 kB
3.574 kB
8.579 kB
3.585 kB
33 B
11 B
react-link
Link
11.422 kB
4.648 kB
11.455 kB
4.66 kB
33 B
12 B
react-select
Select
17.132 kB
6.365 kB
17.183 kB
6.38 kB
51 B
15 B
react-spinner
Spinner
17.795 kB
5.951 kB
17.824 kB
5.956 kB
29 B
5 B
react-text
Text - Default
10.904 kB
4.298 kB
10.937 kB
4.31 kB
33 B
12 B
react-text
Text - Wrappers
14.224 kB
4.719 kB
14.253 kB
4.73 kB
29 B
11 B
react-textarea
Textarea
21.218 kB
7.176 kB
21.269 kB
7.187 kB
51 B
11 B
Unchanged fixtures
Package & Exports Size (minified/GZIP)
priority-overflow
createOverflowManager
2.936 kB
1.212 kB
react-overflow
hooks only
10.792 kB
4.124 kB
react-portal
Portal
6.272 kB
2.17 kB
react-positioning
usePopper
23.21 kB
8.084 kB
react-theme
Single theme token import
69 B
89 B
react-theme
Teams: all themes
31.363 kB
7.043 kB
react-theme
Teams: Light theme
19.806 kB
5.699 kB
react-utilities
SSRProvider
189 B
161 B
🤖 This report was generated against 1c7023ccb89fd70eea5c8084e40786981b7df172

@size-auditor
Copy link

size-auditor bot commented May 16, 2022

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: 1c7023ccb89fd70eea5c8084e40786981b7df172 (build)

@fabricteam
Copy link
Collaborator

fabricteam commented May 16, 2022

Perf Analysis (@fluentui/react-northstar)

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
AlertMinimalPerf.default 225 195 1.15:1
CardMinimalPerf.default 461 401 1.15:1
AccordionMinimalPerf.default 123 109 1.13:1
AvatarMinimalPerf.default 155 138 1.12:1
StatusMinimalPerf.default 551 492 1.12:1
MenuButtonMinimalPerf.default 1429 1283 1.11:1
TreeMinimalPerf.default 678 615 1.1:1
VideoMinimalPerf.default 559 510 1.1:1
AttachmentMinimalPerf.default 131 120 1.09:1
RosterPerf.default 908 831 1.09:1
ButtonMinimalPerf.default 131 121 1.08:1
SegmentMinimalPerf.default 301 280 1.08:1
TableMinimalPerf.default 333 307 1.08:1
TextMinimalPerf.default 267 247 1.08:1
BoxMinimalPerf.default 287 267 1.07:1
LabelMinimalPerf.default 303 284 1.07:1
ItemLayoutMinimalPerf.default 961 909 1.06:1
ProviderMergeThemesPerf.default 1073 1012 1.06:1
ListWith60ListItems.default 508 482 1.05:1
LayoutMinimalPerf.default 305 293 1.04:1
ListCommonPerf.default 518 498 1.04:1
CheckboxMinimalPerf.default 2227 2168 1.03:1
DialogMinimalPerf.default 652 634 1.03:1
DividerMinimalPerf.default 294 285 1.03:1
DropdownManyItemsPerf.default 574 557 1.03:1
FormMinimalPerf.default 338 327 1.03:1
ImageMinimalPerf.default 316 307 1.03:1
LoaderMinimalPerf.default 572 555 1.03:1
RefMinimalPerf.default 199 194 1.03:1
TableManyItemsPerf.default 1598 1559 1.03:1
ChatDuplicateMessagesPerf.default 251 246 1.02:1
EmbedMinimalPerf.default 3451 3376 1.02:1
SliderMinimalPerf.default 1425 1392 1.02:1
TextAreaMinimalPerf.default 407 400 1.02:1
ButtonOverridesMissPerf.default 1272 1257 1.01:1
ButtonSlotsPerf.default 456 452 1.01:1
FlexMinimalPerf.default 234 231 1.01:1
HeaderMinimalPerf.default 298 294 1.01:1
MenuMinimalPerf.default 708 704 1.01:1
RadioGroupMinimalPerf.default 362 360 1.01:1
SkeletonMinimalPerf.default 290 286 1.01:1
SplitButtonMinimalPerf.default 3440 3404 1.01:1
CustomToolbarPrototype.default 2269 2239 1.01:1
AnimationMinimalPerf.default 446 447 1:1
AttachmentSlotsPerf.default 911 907 1:1
DropdownMinimalPerf.default 2555 2555 1:1
HeaderSlotsPerf.default 614 611 1:1
ListNestedPerf.default 402 404 1:1
ProviderMinimalPerf.default 335 335 1:1
GridMinimalPerf.default 272 274 0.99:1
ListMinimalPerf.default 421 427 0.99:1
ChatWithPopoverPerf.default 314 319 0.98:1
InputMinimalPerf.default 1062 1082 0.98:1
PopupMinimalPerf.default 506 517 0.98:1
IconMinimalPerf.default 507 515 0.98:1
ToolbarMinimalPerf.default 769 788 0.98:1
TreeWith60ListItems.default 132 135 0.98:1
ChatMinimalPerf.default 597 615 0.97:1
TooltipMinimalPerf.default 911 943 0.97:1
DatepickerMinimalPerf.default 4649 4849 0.96:1
ReactionMinimalPerf.default 280 302 0.93:1
PortalMinimalPerf.default 129 141 0.91:1
CarouselMinimalPerf.default 349 395 0.88:1

@fabricteam
Copy link
Collaborator

fabricteam commented May 16, 2022

Perf Analysis (@fluentui/react)

Scenario Render type Master Ticks PR Ticks Iterations Status
TeachingBubble mount 47490 89297 5000 Possible regression
All results

Scenario Render type Master Ticks PR Ticks Iterations Status
BaseButton mount 802 704 5000
Breadcrumb mount 2331 2165 1000
Checkbox mount 1181 1158 5000
CheckboxBase mount 1070 1085 5000
ChoiceGroup mount 3627 3731 5000
ComboBox mount 851 745 1000
CommandBar mount 8788 8924 1000
ContextualMenu mount 10176 10392 1000
DefaultButton mount 1005 873 5000
DetailsRow mount 3102 3141 5000
DetailsRowFast mount 3058 3076 5000
DetailsRowNoStyles mount 2957 3071 5000
Dialog mount 1830 1732 1000
DocumentCardTitle mount 141 153 1000
Dropdown mount 2731 2750 5000
FocusTrapZone mount 1580 1451 5000
FocusZone mount 1517 1558 5000
IconButton mount 1383 1377 5000
Label mount 268 302 5000
Layer mount 2395 2411 5000
Link mount 403 373 5000
MenuButton mount 1215 1127 5000
MessageBar mount 1660 1802 5000
Nav mount 2516 2799 1000
OverflowSet mount 926 896 5000
Panel mount 1755 1841 1000
Persona mount 787 832 1000
Pivot mount 1235 1120 1000
PrimaryButton mount 1054 1064 5000
Rating mount 6236 6302 5000
SearchBox mount 1017 1064 5000
Shimmer mount 2176 2030 5000
Slider mount 1555 1678 5000
SpinButton mount 3994 4289 5000
Spinner mount 342 334 5000
SplitButton mount 2563 2410 5000
Stack mount 401 455 5000
StackWithIntrinsicChildren mount 1864 1886 5000
StackWithTextChildren mount 4252 4223 5000
SwatchColorPicker mount 9368 9542 5000
TagPicker mount 2276 2187 5000
TeachingBubble mount 47490 89297 5000 Possible regression
Text mount 368 315 5000
TextField mount 1238 1169 5000
ThemeProvider mount 973 1043 5000
ThemeProvider virtual-rerender 488 564 5000
ThemeProvider virtual-rerender-with-unmount 1516 1511 5000
Toggle mount 662 708 5000
buttonNative mount 97 103 5000

@Hotell Hotell changed the title Hotell/ts/improve react utils types feat(react-utilities): make dicionaries strict and get rid of unused generics to improve DX and type safety May 16, 2022
@khmakoto khmakoto changed the title feat(react-utilities): make dicionaries strict and get rid of unused generics to improve DX and type safety feat(react-utilities): make dictionaries strict and get rid of unused generics to improve DX and type safety May 19, 2022
@fabricteam
Copy link
Collaborator

fabricteam commented May 20, 2022

Perf Analysis (@fluentui/react-components)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 947 908 5000
Button mount 586 596 5000
FluentProvider mount 1896 1990 5000
FluentProviderWithTheme mount 292 300 10
FluentProviderWithTheme virtual-rerender 243 224 10
FluentProviderWithTheme virtual-rerender-with-unmount 295 339 10
MakeStyles mount 1706 1706 50000

@@ -62,15 +62,15 @@ export const useMenuPopover_unstable = (props: MenuPopoverProps, ref: React.Ref<

const { onMouseEnter: onMouseEnterOriginal, onKeyDown: onKeyDownOriginal } = rootProps;

rootProps.onMouseEnter = useEventCallback((e: React.MouseEvent<HTMLElement>) => {
rootProps.onMouseEnter = useEventCallback((e: React.MouseEvent<HTMLDivElement>) => {
Copy link
Contributor Author

@Hotell Hotell May 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because we now have strict type checking and inference, we need to provide correct types instead of wide types

@@ -56,7 +55,7 @@ export const useButton_unstable = (
type: 'button', // This is added because the default for type is 'submit'
},
}),
),
) as ButtonState['root'],
Copy link
Contributor Author

@Hotell Hotell May 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we now have proper getNativeProps types, elements that can have variadic root cannot match with original Props definition where Slot mapped type is used to define the api.

Why?

Slot api design is cannot match with implementation/runtime - Slot uses binary arguments, where 1st generic can only be a 1st string. from component API perspective that's impossible to achieve as Slot is always 1 string or union of multiple strings -> we cannot extract first type from union nor transform unions to arrays.

// impossible transform in TypeScript ❌

type Element = 'div' | 'span' | 'section' 

↓↓↓

type SlotCompatibleType = <'div' , 'span' | 'section'> 

{
'data-automation-id': 1,
},
divProperties,
divProperties as typeof divProperties & {
Copy link
Contributor Author

@Hotell Hotell May 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because our TS version doesn't support tagged literal as index types we cannot type this properly.

Do we wanna move forward with this (Breaking change) or we should go back to following:

Q: can we determine if partners use this function in their codebase ? We export it from react-components so it's a public API

// Implementation with type unaware Allowed Prop Names
// - this will not return proper type but will allow/resolve data-* aria-*  properly
// - basically only provided ExcludedProps will properly resolve the return prop
declare function getNativeProps<
  Props extends Record<string, unknown>,
  E extends Extract<keyof Props, string> = never
>(
  props: Props,
  allowedPropNames: string[] | Record<string, 1>,
  excludedPropNames?: E[],
): Omit<Props, E>

// Example:
// $ExpectType {a:number, c:number} - actual return value is  {a:number} 🆘 
const result = getNativeProps({ a: 1, b: 2, c: 3 }, ['a', 'b'], ['b']);

@msft-fluent-ui-bot
Copy link
Collaborator

Because this pull request has not had activity for over 150 days, we're automatically closing it for house-keeping purposes.

The pull request will still be available for reference. If it's still relevant to merge at some point, you can reopen or make a new version based on the latest code.

@msft-fluent-ui-bot msft-fluent-ui-bot added the Resolution: Soft Close Soft closing inactive issues over a certain period label Oct 22, 2022
@Hotell Hotell reopened this Oct 24, 2022
@msft-fluent-ui-bot
Copy link
Collaborator

Because this pull request has not had activity for over 150 days, we're automatically closing it for house-keeping purposes.

The pull request will still be available for reference. If it's still relevant to merge at some point, you can reopen or make a new version based on the latest code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fluent UI react-components (v9) Resolution: Soft Close Soft closing inactive issues over a certain period
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants